Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add AsdfProvisionalAPIWarning #1295

Merged

Conversation

braingram
Copy link
Contributor

This warning should be used for any experimental api changes that should be considered provisional and might experience breaking changes (even in minor releases).

@braingram
Copy link
Contributor Author

braingram commented Jan 10, 2023

These changes taken from #1290 which is a candidate for using this warning.

Other new features that are candidates for using this warning are:

@braingram braingram marked this pull request as ready for review January 10, 2023 18:56
@braingram braingram requested a review from a team as a code owner January 10, 2023 18:56
@WilliamJamieson
Copy link
Contributor

Can we create a decorator so that we can just do something like:

@asdf_provisional
def some_new_api(*args, **kwargs):
	pass

Instead of having to do all the raise warning stuff?

@braingram
Copy link
Contributor Author

A decorator that issues the warning might be nice. How would we handle #1289 where the change is to an existing method?

@WilliamJamieson
Copy link
Contributor

WilliamJamieson commented Jan 10, 2023

I think a decorator like I am thinking of would look like:

def asdf_provisional(func, msg=None):
    msg = f"{func.__name__} has a provisional API, changes can happen at any time!" if msg is None else msg

    def wrapper(*args, **kwargs):
        warnings.warn(msg, AsdfProvisionalAPIWarning)

       func(*args, **kwargs)

    return wrapper

@WilliamJamieson
Copy link
Contributor

WilliamJamieson commented Jan 10, 2023

A decorator that issues the warning might be nice. How would we handle #1289 where the change is to an existing method?

I think we can model https://github.com/astropy/astropy/blob/e09ed4b9cddd48807564e498d3a21f16719c0e50/astropy/utils/decorators.py#L325 from astropy, only instead of it being a depreciation it is instead the provisional warning.

@braingram
Copy link
Contributor Author

Thanks for sending the astropy link. Do we need something that complex? Perhaps I'm not fully understanding what this warning will be used for.

@WilliamJamieson
Copy link
Contributor

It doesn't need to be that complex. I was mostly thinking of adding it as a wrapper on new functions like I showed above. The astropy example is just something we could adapt (simplify) to meet our needs.

@braingram
Copy link
Contributor Author

I agree that something simpler than the linked astropy code is preferred.

For the 3 PRs that could use this warning:

For #1287 this looks like a reasonable place for a warning:

if hasattr(self.delegate, "validators"):
return self.delegate.validators
return {}

we could call warnings.warn right before line 408 or adapt deprecated_attribute from astropy.

For #1289 we could call warnings.warn if tree_mapper is not None:

asdf/asdf/asdf.py

Lines 871 to 872 in 802b759

if tree_mapper is not None:
tree = tree_mapper(tree)

or we could implement something like deprecated_renamed_argument (does this handle new arguments?)

For #1290 we could replace the generic warning with the ASDFProvisionalAPIWarning one:

asdf/asdf/asdf.py

Lines 1856 to 1861 in 7e50d02

@property
def block_manager(self):
warnings.warn(
"The block_manager API within a SerializationContext is not yet stable", AsdfProvisionalAPIWarning
)
return self._block_manager

or use something like deprecated_attribute.

We might also consider using a provisional version of reserve_blocks to address: astropy/asdf-astropy#156 As there's no PR to fix this it's hard to predict what construct we will want to decorate.

Are there other uses I'm missing?

@braingram
Copy link
Contributor Author

Thanks for the PR on the source branch for this PR:
braingram#2
I don't see why we need something more complex then warnings.warn. For the uses previously mentioned and the changes you suggested:

Looking at #1287 again the provisional_attribute approach won't work here as all subclasses of Extension will provide validators (falling back to the default blank dict). One option would be to check if the provided validators is empty and if not, issue a warning.

For #1289 we could decorate asdf.open_asdf with provisional_argument to cover calls to asdf.open_asdf, asdf.AsdfFile.open (deprecated) and asdf.AsdfFile.open_external. I think these are all the public ways open an asdf file. Decorating a lower level function like asdf.AsdfFile._open_asdf doesn't work because tree_mapper is provided (as None) so the warning is always issued. We could also issue a warning where tree_mapper is used to make sure we catch all uses.

For #1290 it looks like for provisional_attribute we would need to add a class attribute to SerializationContext:

    block_manager = provisional_attribute("block_manager")

then assign to SerailzationContext._block_manager on init (to avoid issuing the warning every time). I got this wrong several times (thinking the provisional_attribute should decorate the property, etc) so is there a way to use this that I overlooked? With the above change, it would be possible for an extension to overwrite the block manager (using the provisional_attribute setter). The code in #1290 doesn't currently allow for this (no setter is defined) and if we want to keep that feature we could instead add a warning to the existing property.

As warnings.warn covers all our current uses my preference is to use it.

An alternative to introducing a provisional api at all would be to just push development over to a 3.0 branch and add/change features without fear of breaking things. As the provisional api won't handle removal of features (like AsdfInFits in #1288) we will likely need to move to 3.0. I wouldn't object to shelving the provisional api and moving all the applicable changes to a 3.0 branch.

@WilliamJamieson
Copy link
Contributor

Thanks for the PR on the source branch for this PR: braingram#2 I don't see why we need something more complex then warnings.warn. For the uses previously mentioned and the changes you suggested:

The complexity if provisional occurs so that there is an optional custom message, rather than requiring a custom message or using the same message call.

Looking at #1287 again the provisional_attribute approach won't work here as all subclasses of Extension will provide validators (falling back to the default blank dict). One option would be to check if the provided validators is empty and if not, issue a warning.

It won't be able to cover all cases, such as this one.

For #1289 we could decorate asdf.open_asdf with provisional_argument to cover calls to asdf.open_asdf, asdf.AsdfFile.open (deprecated) and asdf.AsdfFile.open_external. I think these are all the public ways open an asdf file. Decorating a lower level function like asdf.AsdfFile._open_asdf doesn't work because tree_mapper is provided (as None) so the warning is always issued. We could also issue a warning where tree_mapper is used to make sure we catch all uses.

I don't understand the point of decorating part of the private API like _open_asdf. Its the private API so we make no guarantees about its functionality.

For #1290 it looks like for provisional_attribute we would need to add a class attribute to SerializationContext:

    block_manager = provisional_attribute("block_manager")

then assign to SerailzationContext._block_manager on init (to avoid issuing the warning every time). I got this wrong several times (thinking the provisional_attribute should decorate the property, etc) so is there a way to use this that I overlooked? With the above change, it would be possible for an extension to overwrite the block manager (using the provisional_attribute setter). The code in #1290 doesn't currently allow for this (no setter is defined) and if we want to keep that feature we could instead add a warning to the existing property.

The API for provisional_attribute can be modified to control if a setter and deleter are included. I can also modify it so that it can be run by the __init__ and accept an initial value.

As warnings.warn covers all our current uses my preference is to use it.

This is fine, I prefer the decorator approach as it should amount to just adding an @provisional on new functions or @provisional_argument for new arguments to existing functions. provisioinal_attribute was an experiment on my part to see if I get it to work.

The other reason I prefer the decorator/convenience API here is that I think we might want tweak the category and/or stacklevel of all the provisional API warnings. This is simpler to do if all of the actual uses of the warning are limited to a central place.

All this being said feel free to reject my PR we just need to be consistent in however we construct the provisional API.

An alternative to introducing a provisional api at all would be to just push development over to a 3.0 branch and add/change features without fear of breaking things. As the provisional api won't handle removal of features (like AsdfInFits in #1288) we will likely need to move to 3.0. I wouldn't object to shelving the provisional api and moving all the applicable changes to a 3.0 branch.

I strongly disagree with doing this. I personally think we should keep all this new API as provisional for at least one major release. Its fine to assume it doesn't get released until 3.0, but provisional status should not be removed until at least 4.0 in that case. The whole point of the provisional status was to enable us to release ASDF with the API so that we can use it and tweak it as we learn without worrying about backwards compatiblity concerns. As the API stabilizes we can remove the provisional status of components.

@braingram
Copy link
Contributor Author

An alternative to introducing a provisional api at all would be to just push development over to a 3.0 branch and add/change features without fear of breaking things. As the provisional api won't handle removal of features (like AsdfInFits in #1288) we will likely need to move to 3.0. I wouldn't object to shelving the provisional api and moving all the applicable changes to a 3.0 branch.

I strongly disagree with doing this. I personally think we should keep all this new API as provisional for at least one major release. Its fine to assume it doesn't get released until 3.0, but provisional status should not be removed until at least 4.0 in that case. The whole point of the provisional status was to enable us to release ASDF with the API so that we can use it and tweak it as we learn without worrying about backwards compatiblity concerns. As the API stabilizes we can remove the provisional status of components.

Thanks for clarifying this. I did not understand that this was what you proposed during the asdf tag up. So that I'm understanding what you're proposing, are you saying that any new feature or api change we want to add:

  • should be marked provisional and added to any upcoming 2.x releases
  • be included in 3.0 but kept as provisional
  • have provisional warnings removed (for whatever we deem is stable) in version 4.0

@WilliamJamieson
Copy link
Contributor

Thanks for clarifying this. I did not understand that this was what you proposed during the asdf tag up. So that I'm understanding what you're proposing, are you saying that any new feature or api change we want to add:

  • should be marked provisional and added to any upcoming 2.x releases
  • be included in 3.0 but kept as provisional
  • have provisional warnings removed (for whatever we deem is stable) in version 4.0

Essentially, yes modulo how we decided to do versions (we may want to do 3.0 as just remove asdf-in-fits in which case we bump what you said by 1). I want all the experimental/new stuff to appear as provisional for a major release cycle and hopefully be finalized as part of the next major release.

This warning should be used for any experimental api changes
that should be considered provisional and might experience breaking
changes (even in minor releases).
Copy link
Contributor

@WilliamJamieson WilliamJamieson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just manually raising the warning is fine for now.

@WilliamJamieson WilliamJamieson merged commit 6e04523 into asdf-format:master Jan 20, 2023
@braingram braingram deleted the feature/provisional_warning branch January 20, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants